PCSX2 injection support for vanilla JP KH1, vanilla USA KH1 and JP ReCoM#1255
PCSX2 injection support for vanilla JP KH1, vanilla USA KH1 and JP ReCoM#1255shananas merged 8 commits intoOpenKH:masterfrom
Conversation
Mentioning the new JP ReCoM support.
Accept JP ReCoM ISO.
Added JP ReCoM support. Some instruction parameters had to be changed in the hook functions so I duplicated them. A more elegant solution is welcome.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds JP-region detectors for KH1/Recom, extends PCSX2 injector offsets/hooks with JP and vanilla variants and conditional injection, adjusts Recom ISO sector calculation, and updates SetupWizard supported-game labels. Changes
Sequence Diagram(s)(none — changes are metadata, offsets and a small extraction tweak; flow remains localized and not requiring a multi-component sequence diagram.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
OpenKh.Tools.ModsManager/Services/Pcsx2Injector.cs (1)
487-546: Consider parameterizing JP/US hook generation to avoid drift.These two JP arrays are near-duplicates of existing hooks with shifted immediates. A small builder method would keep behavior identical and reduce maintenance risk when hooks evolve.
♻️ Refactor sketch
- private static readonly uint[] LoadFileAsyncHook = new uint[] { ... }; - private static readonly uint[] LoadFileAsyncHookJp = new uint[] { ... }; + private static uint[] BuildLoadFileAsyncHook(short oA, short oB, short oC, short oD) => new uint[] + { + LUI(T6, HookStack), + LW(T2, V0, oA), + LW(T3, V0, oB), + ADDI(T4, V0, oC), + ... + LW(A2, V0, oD), + ... + }; + private static readonly uint[] LoadFileAsyncHook = BuildLoadFileAsyncHook(-0x2A10, -0x2A24, -0x29F8, -0x29F4); + private static readonly uint[] LoadFileAsyncHookJp = BuildLoadFileAsyncHook(0x18B0, 0x189C, 0x18C8, 0x18CC); - private static readonly uint[] GetFileSizeRecomHook = new uint[] { ... }; - private static readonly uint[] GetFileSizeRecomHookJp = new uint[] { ... }; + private static uint[] BuildGetFileSizeRecomHook(short fileDirOffset) => new uint[] + { + LUI(T6, HookStack), + LUI(V1, 0x5C), + ADDI(T4, V1, fileDirOffset), + ... + }; + private static readonly uint[] GetFileSizeRecomHook = BuildGetFileSizeRecomHook(-0x29F8); + private static readonly uint[] GetFileSizeRecomHookJp = BuildGetFileSizeRecomHook(0x18C8);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@OpenKh.Tools.ModsManager/Services/Pcsx2Injector.cs` around lines 487 - 546, The two JP hook arrays (LoadFileAsyncHookJp and GetFileSizeRecomHookJp) are near-duplicates of US hooks with only immediate/address shifts; create a small builder method (e.g., BuildHookSequence or BuildJpHook) that accepts the varying immediates/offsets (V1 high words like 0x5B/0x5C, the AddI/AddIU immediates such as 0x18C8/0x18CC, and any differing constant like Operation.GetFileSizeRecom) and returns the uint[] of instructions using the same instruction helpers (LUI, LW, SW, BEQ, BNE, ADDI, ADDIU, JR, NOP, etc.); replace the hardcoded arrays LoadFileAsyncHookJp and GetFileSizeRecomHookJp with calls to that builder passing the specific constants (maintain use of HookStack, Param1/Param2/ParamOperator/ParamReturn, and registers T0..T6, V0/V1, S1/S2, RA) so behavior stays identical while preventing drift when the common hook template changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@OpenKh.Tools.ModsManager/Services/Pcsx2Injector.cs`:
- Around line 487-546: The two JP hook arrays (LoadFileAsyncHookJp and
GetFileSizeRecomHookJp) are near-duplicates of US hooks with only
immediate/address shifts; create a small builder method (e.g., BuildHookSequence
or BuildJpHook) that accepts the varying immediates/offsets (V1 high words like
0x5B/0x5C, the AddI/AddIU immediates such as 0x18C8/0x18CC, and any differing
constant like Operation.GetFileSizeRecom) and returns the uint[] of instructions
using the same instruction helpers (LUI, LW, SW, BEQ, BNE, ADDI, ADDIU, JR, NOP,
etc.); replace the hardcoded arrays LoadFileAsyncHookJp and
GetFileSizeRecomHookJp with calls to that builder passing the specific constants
(maintain use of HookStack, Param1/Param2/ParamOperator/ParamReturn, and
registers T0..T6, V0/V1, S1/S2, RA) so behavior stays identical while preventing
drift when the common hook template changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f06c1840-0840-438b-96d1-5ac5e33bbebf
📒 Files selected for processing (3)
OpenKh.Tools.ModsManager/Services/GameService.csOpenKh.Tools.ModsManager/Services/Pcsx2Injector.csOpenKh.Tools.ModsManager/Views/SetupWizardWindow.xaml
Both vanilla JP and USA KH1 needs a smaller skip jump than KH1FM. I did the same duplicate method as for JP ReCoM.
Vanilla JP and USA versions of KH1 are now valid KH1 ISOs for the ISO selector.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
OpenKh.Tools.ModsManager/Services/Pcsx2Injector.cs (2)
938-945: Consider extracting repeated hook-injection boilerplate.The three new conditional blocks repeat the same
Log.Info+WritePatch(... JAL(WriteHook(...)), ADDIU(...))pattern. A small helper would reduce copy/paste risk as more variants are added.Also applies to: 955-961, 998-1004
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@OpenKh.Tools.ModsManager/Services/Pcsx2Injector.cs` around lines 938 - 945, Extract the repeated injection boilerplate into a small helper method (e.g., InjectHook) and replace the three repeated blocks with calls to it; the helper should take the stream, the target offset (like offsets.LoadFileTaskVanilla), a hook writer/delegate (like LoadFileTaskHookVanilla passed to WriteHook), the operation enum value (Operation.LoadFileTask) and a name for logging, then perform the Log.Info and the WritePatch(stream, offset, ADDIU(...), JAL(WriteHook(...)), ADDIU(...)) sequence using WriteHook and WritePatch so callers simply call InjectHook(stream, offsets.LoadFileTaskVanilla, LoadFileTaskHookVanilla, Operation.LoadFileTask, nameof(offsets.LoadFileTaskVanilla)); update the other occurrences (lines referencing offsets.* and their respective Hook and Operation) to use this helper.
427-455: ParameterizeLoadFileTaskhook construction to avoid code drift.
LoadFileTaskHookandLoadFileTaskHookVanillaare identical except the skip immediate at Line 451 (0x64vs0x98in the other hook). This is a good candidate for a small hook-builder helper.♻️ Suggested refactor
- private static readonly uint[] LoadFileTaskHook = new uint[] - { - ... - ADDIU(RA, RA, 0x98), - ... - }; - - private static readonly uint[] LoadFileTaskHookVanilla = new uint[] - { - ... - ADDIU(RA, RA, 0x64), - ... - }; + private static uint[] BuildLoadFileTaskHook(short skipImmediate) => new uint[] + { + LUI(T6, HookStack), + SW(S1, T6, Param1), + SW(S0, T6, Param2), + SW(V0, T6, Param3), + SW(T5, T6, ParamOperator), + LW(T5, T6, ParamOperator), + BNE(T5, (byte)Operation.HookExit, -2), + LW(V1, T6, ParamReturn), + BEQ(V1, Zero, 3), + MOVE(S2, V0), + BEQ(Zero, Zero, 2), + ADDIU(RA, RA, skipImmediate), + LI(V0, -1), + JR(RA), + NOP(), + }; + + private static readonly uint[] LoadFileTaskHook = BuildLoadFileTaskHook(0x98); + private static readonly uint[] LoadFileTaskHookVanilla = BuildLoadFileTaskHook(0x64);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@OpenKh.Tools.ModsManager/Services/Pcsx2Injector.cs` around lines 427 - 455, The two hook arrays LoadFileTaskHook and LoadFileTaskHookVanilla differ only by the skip immediate (ADDIU(RA, RA, 0x64) vs 0x98); refactor by extracting a hook-builder (e.g., BuildLoadFileTaskHook or CreateLoadFileTaskHook) that returns the uint[] and takes the skipImmediate as a parameter, then replace both LoadFileTaskHook and LoadFileTaskHookVanilla with calls to that builder (pass 0x64 and 0x98 respectively); ensure the builder reproduces the exact sequence of instructions (LUI(T6, HookStack), SW(...), LW(...), BNE(...), LW(V1,...), BEQ(...), MOVE(...), BEQ(...), ADDIU(RA, RA, skipImmediate), LI(V0, -1), JR(RA), NOP()) and retains the same identifiers (T6, S1, S0, V0, T5, V1, S2, RA) so all references remain valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@OpenKh.Tools.ModsManager/Services/Pcsx2Injector.cs`:
- Around line 938-945: Extract the repeated injection boilerplate into a small
helper method (e.g., InjectHook) and replace the three repeated blocks with
calls to it; the helper should take the stream, the target offset (like
offsets.LoadFileTaskVanilla), a hook writer/delegate (like
LoadFileTaskHookVanilla passed to WriteHook), the operation enum value
(Operation.LoadFileTask) and a name for logging, then perform the Log.Info and
the WritePatch(stream, offset, ADDIU(...), JAL(WriteHook(...)), ADDIU(...))
sequence using WriteHook and WritePatch so callers simply call
InjectHook(stream, offsets.LoadFileTaskVanilla, LoadFileTaskHookVanilla,
Operation.LoadFileTask, nameof(offsets.LoadFileTaskVanilla)); update the other
occurrences (lines referencing offsets.* and their respective Hook and
Operation) to use this helper.
- Around line 427-455: The two hook arrays LoadFileTaskHook and
LoadFileTaskHookVanilla differ only by the skip immediate (ADDIU(RA, RA, 0x64)
vs 0x98); refactor by extracting a hook-builder (e.g., BuildLoadFileTaskHook or
CreateLoadFileTaskHook) that returns the uint[] and takes the skipImmediate as a
parameter, then replace both LoadFileTaskHook and LoadFileTaskHookVanilla with
calls to that builder (pass 0x64 and 0x98 respectively); ensure the builder
reproduces the exact sequence of instructions (LUI(T6, HookStack), SW(...),
LW(...), BNE(...), LW(V1,...), BEQ(...), MOVE(...), BEQ(...), ADDIU(RA, RA,
skipImmediate), LI(V0, -1), JR(RA), NOP()) and retains the same identifiers (T6,
S1, S0, V0, T5, V1, S2, RA) so all references remain valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5745a92-2026-4e9c-8a8d-77b79dc33084
📒 Files selected for processing (1)
OpenKh.Tools.ModsManager/Services/Pcsx2Injector.cs
Mention vanilla USA and JP KH1 support.
|
In the 3 commits I made today I added support for the vanilla USA and JP versions of KH1. |
|
I did some testing with the JP RECOM and JP & US KH1 ISO's. File Extraction Test:
File Injection Test:
File Fallback Test:
Everything seemed to work except that I was unable to extract the JP RECOM. If you could address that issue, I will give my approval. |
|
I found what causes the issue with the extraction. There is an off by one error when determining the size of the memory buffer using the sector count for the JP version. This can be resolved by changing line 93 of OpenKh.Tools.ModsManager/Services/GameDataExtractionService.cs to |
I tested with the USA ROM and it doesn't seem to break it but I only cancelled the extraction after a short while. some1fromthedark's fix.
|
I added your fix and it doesn't seem to break USA extraction. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@OpenKh.Tools.ModsManager/Services/GameDataExtractionService.cs`:
- Line 93: The code reading the RDI sector count uses a single-byte read at the
sector start; update the call that creates rdi_stream (the IsoUtility.GetSectors
call) to read the 16-bit sector-count field at offset +0x8 from the sector
start, add 1 to that 16-bit value, and pass that resulting count to GetSectors.
Locate the expression that uses stream.SetPosition(0x244 * 0x800).ReadByte() and
replace it with logic that seeks to (0x244 * 0x800) + 0x8, reads a 16-bit
unsigned integer (proper endianness as used elsewhere in this project), adds 1,
and uses that value for the third parameter to IsoUtility.GetSectors so
rdi_stream is sized correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 838a5ec2-3ff1-4c8b-b506-f7ea74523343
📒 Files selected for processing (3)
OpenKh.Tools.ModsManager/Services/GameDataExtractionService.csOpenKh.Tools.ModsManager/Services/GameService.csOpenKh.Tools.ModsManager/Views/SetupWizardWindow.xaml
✅ Files skipped from review due to trivial changes (1)
- OpenKh.Tools.ModsManager/Views/SetupWizardWindow.xaml
🚧 Files skipped from review as they are similar to previous changes (1)
- OpenKh.Tools.ModsManager/Services/GameService.cs
Sorry, that was on me for not copying the line. Weirdly enough that worked too. This works with both ROMs.
Some1fromthedark
left a comment
There was a problem hiding this comment.
All of the tests I ran in my previous comment are now passing. LGTM.
I added the necessary changes to the Mod Managers code so the PCSX2 injection feature is compatible with the original Japanese PS2 release of ReCoM.
The 2 offsets had to be shifted with +0x90 while the negative instruction parameters in the 2 hook functions with +0x42C0.
I didn't want to touch the structure of the file so for now I duplicated the 2 hook functions but I propose an offset parameter based on the USA release since that was added first.
Summary by CodeRabbit
New Features
Bug Fixes
UI Updates